Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Security solution] [RAC] Add row renderer popover to alert table "reason" field #108054

Merged
merged 3 commits into from
Aug 12, 2021
Merged

[Security solution] [RAC] Add row renderer popover to alert table "reason" field #108054

merged 3 commits into from
Aug 12, 2021

Conversation

machadoum
Copy link
Member

@machadoum machadoum commented Aug 10, 2021 •
edited
Loading

Summary

  • It displays a popover containing the row renderer when the user clicks on the reason field value.
  • It creates a new column renderer (reason_column_renderer.tsx) for the reason field that renders the row renderer on click.
  • It passes down the props required for the reason_column_renderer to call row renderer.

Screenshot 2021-08-11 at 17 10 22

Screenshot 2021-08-11 at 17 10 36

Checklist

  • Unit or functional tests were updated or added to match the most common scenarios
  • Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
  • Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)

@machadoum machadoum changed the title Add row renderer popover to user.name alert table field [Security solution] [RAC] Add row renderer popover to "user.name" alert table field Aug 10, 2021
@machadoum machadoum self-assigned this Aug 10, 2021
@machadoum machadoum added Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.15.0 v8.0.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Aug 10, 2021
@mdefazio
Copy link
Contributor

Do we need to include the event render type in the title of the popover? I'm not super knowledgable about how all these differ and how useful that is to know.

@machadoum
Copy link
Member Author

Do we need to include the event render type in the title of the popover? I'm not super knowledgable about how all these differ and how useful that is to know.

I also don't know how useful it would be. The event renderer name column isn't very informative:
Screenshot 2021-08-11 at 13 15 16

Maybe @paulewing can help us with that.

@machadoum machadoum changed the title [Security solution] [RAC] Add row renderer popover to "user.name" alert table field [Security solution] [RAC] Add row renderer popover to alert table "reason" field Aug 11, 2021
@mdefazio
Copy link
Contributor

The more I think about it, at the very least, I do think we should include a title of 'Event renderer'. When we bring in the view of all these event renderers, we will want that correlation between the two.

I will discuss with @paulewing whether we need a title, but even then, I think it's better to do something like:
Event renderer: {event_renderer_name}.

@mdefazio
Copy link
Contributor

Spoke with @paulewing , let's go with Event renderer: {event_renderer_name} for the popover title

@machadoum machadoum force-pushed the rac-event-renderer-popup branch 2 times, most recently from d0cc470 to f97f934 Compare August 11, 2021 15:24
@michaelolo24 michaelolo24 mentioned this pull request Aug 11, 2021
Closed
48 tasks
@mdefazio mdefazio requested a review from a team August 11, 2021 19:39
@michaelolo24
Copy link
Contributor

@elasticmachine merge upstream

@machadoum machadoum marked this pull request as ready for review August 12, 2021 09:46
@machadoum machadoum requested review from a team as code owners August 12, 2021 09:46
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@machadoum
Copy link
Member Author

Spoke with @paulewing , let's go with Event renderer: {event_renderer_name} for the popover title

I added a new commit with the title change.
Screenshot 2021-08-12 at 11 47 36

@michaelolo24 The PR is ready for review!

michaelolo24
michaelolo24 reviewed Aug 12, 2021
View reviewed changes
...lugins/security_solution/public/timelines/components/row_renderers_browser/catalog/index.tsx
[RowRendererId.suricata]: 'Suricata',
[RowRendererId.threat_match]: i18n.THREAT_MATCH_NAME,
[RowRendererId.zeek]: i18n.ZEEK_NAME,
[RowRendererId.plain]: '',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want a default name for the plain renderer? We can do that in a follow up PR, but we should find out

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am struggling to find a scenario where we would display the plain RowRenderer. I think that it won't be displayed on the alert page.

When I call getRowRenderer and no ronRenderer is found it returns null Instead of returning plainRowRenderer.

I couldn't find any reference to plain_row_renderer.tsx on the code.

Here there is a comment mentioning it but the comment is outdated because plainRowRenderer is not included on defaultRowRenderers
Screenshot 2021-08-12 at 14 13 54

If I am right, we could delete plain_row_renderer.tsx

@jportner
Copy link
Contributor

It looks like Team:Security was mistakenly added, I'll remove it

@jportner jportner removed the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label Aug 12, 2021
michaelolo24
michaelolo24 reviewed Aug 12, 2021
View reviewed changes
...solution/public/timelines/components/timeline/body/renderers/reason_column_renderer.test.tsx
@@ -0,0 +1,148 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding tests!

@machadoum machadoum added the Team:Threat Hunting Security Solution Threat Hunting Team label Aug 12, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

michaelolo24
michaelolo24 reviewed Aug 12, 2021
View reviewed changes
...rity_solution/public/timelines/components/timeline/body/renderers/reason_column_renderer.tsx Outdated

const rowRenderer = useMemo(() => getRowRenderer(ecsData, rowRenderers), [ecsData, rowRenderers]);

const rowRender =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you update this to:

useMemo(() => {
	return rowRenderer && rowRenderer.renderRow({
      browserFields,
      data: ecsData,
      isDraggable: true,
      timelineId,
    });
}, [..deps]);

michaelolo24
michaelolo24 approved these changes Aug 12, 2021
View reviewed changes
Copy link
Contributor

@michaelolo24 michaelolo24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really looks great! Tested out a couple different renderers such as auditd and netflow and they worked great. Just a few comments, but nothing blocking! Great job! 💪🏾

@machadoum
Copy link
Member Author

@michaelolo24 Thank you for reviewing the PR 🙇‍♂️

I fixed all issues. Feel free to take a second look.

@machadoum machadoum enabled auto-merge (squash) August 12, 2021 13:35
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2360 2361 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 6.5MB 6.5MB +6.3KB
timelines 267.6KB 267.7KB +108.0B
total +6.4KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @machadoum

@machadoum machadoum merged commit 79fefe0 into elastic:master Aug 12, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 12, 2021
@machadoum @kibanamachine
[Security solution] [RAC] Add row renderer popover to alert table "re…
5c2fbf2 
…ason" field (elastic#108054)

* Add row renderer popover to alert table reason field

* Add a title to row renderer popover on alert table

* Fix issues found during code review
@kibanamachine kibanamachine mentioned this pull request Aug 12, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Aug 12, 2021
@kibanamachine @machadoum
[Security solution] [RAC] Add row renderer popover to alert table "re…
ccb3a97 
…ason" field (#108054) (#108385)

* Add row renderer popover to alert table reason field

* Add a title to row renderer popover on alert table

* Fix issues found during code review

Co-authored-by: Pablo Machado <pablo.nevesmachado@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelolo24 michaelolo24 michaelolo24 approved these changes

Assignees

@machadoum machadoum

Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:enhancement Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting Security Solution Threat Hunting Team v7.15.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@machadoum @mdefazio @michaelolo24 @elasticmachine @jportner @kibanamachine